-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update JS bindings with CrossSection + reorganize #440
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #440 +/- ##
=======================================
Coverage 89.67% 89.67%
=======================================
Files 35 35
Lines 4243 4243
=======================================
Hits 3805 3805
Misses 438 438 ☔ View full report in Codecov by Sentry. |
Ah looks like the previously mentioned addition to function debug(shape: Manifold | CrossSection, map: Map<number, Mesh>) {
const manifold = (shape instanceof module.CrossSection)
? shape.extrude(1).translate([0, 0, -0.5])
: shape;
const result = manifold.asOriginal();
map.set(result.originalID(), result.getMesh());
return result;
}; Also, after looking at it again just now, if the cleanup step is required to make sure that Manifolds get freed, is |
Good question - I think that gets cleaned up by the modified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good, but let's clean up the format so I can do a better pass. The examples especially will break pretty badly, since their white space gets processed.
This doesn't go through the modified version does it? It comes before the indirection is set up.maybe if it was moved down the file.
Sure, that doesn't sound bad. |
Oh, good catch! Let's do that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great, thanks! Would you mind pulling master so we can double-check the formatting?
Gyroid example just refusing to not timeout it seems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is ready once the tests pass. Thanks! If you click the merge button, please check when deployment completes and then test ManifoldCAD.org. Since we don't have any staging, I'd like to catch problems early.
Seems like most things are resolved, but I haven't touched the interface files yet which are giving inaccurate/incomplete completion information with all of the static function reorganization and the CrossSection addition unaccounted for. |
bindings/wasm/CMakeLists.txt
Outdated
set_source_files_properties(bindings.cpp | ||
OBJECT_DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/bindings.js | ||
# FIXME: can we rewrite the custom command to run after these are modified when building | ||
# without recompiling bindings.cpp? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, but my CMake foo is weak. Feel free to play around!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that I've got something working for the interface copying that does not require rebuilding the bindings needlessly. 👍
Ah looks like I should have tested the scale cleanup before committing, might have been what broke here. |
- add CrossSection class - move static method (constructors) under their respective classes - add Manifold.{split, splitByPlane}
* | ||
* @param manifolds A list of Manifolds to lazy-union together. | ||
*/ | ||
static compose(manifolds: Manifold[]): Manifold; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: should we put all the static functions next to each other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, I was just organizing them by purpose. I'm not sure what the way that this interface will normally be viewed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably mostly by other maintainers. Just try to make it obvious where someone would put the next method.
As an aside, I have come across a funny bug, the following fails in the editor with const box = CrossSection.square([80, 80], true).offset(10, "Round").extrude(100, 0, 0, [1, 1], false); However, changing the final argument (center) to |
Ugh, WASM errors are so obtuse. Can you repro it in a C++ test? I just fixed some like that for Warp, which turned out to be because I wasn't checking inputs strictly enough. |
I did try doing TEST(CrossSection, RoundedBox) {
auto sq = CrossSection::Square({80, 80}, true)
.Offset(10, CrossSection::JoinType::Round);
auto uncentred = Manifold::Extrude(sq, 100, 0, 0, {1, 1});
auto ball = Manifold::Sphere(60, 100);
auto mesh = (uncentred - ball).GetMesh();
#ifdef MANIFOLD_EXPORT
if (options.exportModels)
ExportMesh("cross_section_rounded_box.glb", mesh, {});
#endif
} It did however take almost 37s to run though. |
Weird, it seems way too simple to take that long. How many facets for "round"? Oh well, we should probably move this discussion to a separate issue anyway. |
Ok. It is a pretty weird problem given than a mere translation makes the difference. This use of offset is bringing it out, but where the error is and how insidious I don't know. |
I finally have a break now. I just have a look at it, there are 281010 edges in the |
Related: http://www.angusj.com/clipper2/Docs/Units/Clipper.Offset/Classes/ClipperOffset/Properties/ArcTolerance.htm |
From how smooth the roundover looks I'm not too surprised. Calculating a default ourselves based on the delta, rather than leaving it up to the Clipper2 default or the user you mean? |
Yeah, we can allow the user to set the |
Caught what I hope to be the last bugs while testing out I remembered a question I'd forgotten around the business in getManifoldDTS in If this stuff is actually (still?) necessary, it seems pretty fragile given that it requires manually copying over the interface into the string block (though it isn't something that happens too often I suppose). |
I believe #348 is where the |
Ok, well I think I'll leave it for another PR then if any problems arise. The types are working for completion and linting in my use of the editor locally at least. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor things and then I think we can merge. Let's try to avoid any more functional changes to this PR so we can close the review cycle. We'll need another one before release anyway to address the offset API.
circle.push([Math.cos(dPhi * i) + offset, Math.sin(dPhi * i)]); | ||
} | ||
const offset = 2. | ||
const circle = CrossSection.circle(1., n).translate([offset, 0.]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for .
in JS - everything is a double - even integers!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ocaml muscle memory acting up again.
const {square} = CrossSection; | ||
const box = square([70, 70], true) | ||
.offset(15, 'Round') | ||
.extrude(100, 0, 0, [1, 1], true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the idea, but I'm intentionally keeping this first landing-page example super simple. Let's revert this for now. We should add a cool example that showcases more of the CrossSection API though at some point.
auto jt = join_type == 0 ? CrossSection::JoinType::Square | ||
: join_type == 1 ? CrossSection::JoinType::Round | ||
: CrossSection::JoinType::Miter; | ||
return cross_section.Offset(delta, jt, miter_limit, arc_tolerance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this in a separate PR, but I agree with @pca006132 that instead of arc_tolerance
we should use our setMinCircularAngle
etc. functions to define this.
const cls = module[className]; | ||
const obj = areStatic ? cls : cls.prototype; | ||
for (const name of methodNames) { | ||
if (name != 'cylinder') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must have missed this in code review - is this intentional? Just want to make sure I can safely remove it now.
love the addition of slice :) |
- add CrossSection class - move static method (constructors) under their respective classes -add Manifold.{split, splitByPlane} - enable usage of instanceof on Manifold (and now CrossSection) objects simplify registry indirection in worker.ts, enabling method de-structuring (opening static function names out of class module name-spaces) Build changes: - break C++ helpers out from bindings.cpp to helpers.cpp - ensure that ts files are copied during builds even when nothing is recompiled Note that in addition to the new bindings, the most of the existing toplevel functions have been moved under their respective classes as static methods (breaking change!). I haven't added any new examples yet, but the existing ones are passing and do make use of CrossSection now (some transparently, and some directly), but I think enough of this is together that review is worthwhile.
Fixes #360
instanceof
onManifold
(and nowCrossSection
) objectsworker.ts
, enabling method de-structuring (opening static function names out of class module name-spaces)Build changes:
bindings.cpp
tohelpers.cpp
ts
files are copied during builds even when nothing is recompiledNote that in addition to the new bindings, the most of the existing toplevel functions have been moved under their respective classes as static methods (breaking change!). I haven't added any new examples yet, but the existing ones are passing and do make use of
CrossSection
now (some transparently, and some directly), but I think enough of this is together that review is worthwhile.